Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat/#5521/derive on selection change and on value change #5523

Conversation

ologbonowiwi
Copy link

@ologbonowiwi ologbonowiwi commented Oct 17, 2023

  • feat: trigger onSelectorChange and onValueChange on onChange
  • docs: add comment
  • docs: add changeset

Description
Keep onChange and add onSelectionChange to listen to selection change only and onValueChange to listen to value change only.

Issue
Fixes: #5521
/claim udecode/plate#2700

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)
  • You've added a changeset if changing functionality. (Add one with yarn changeset add.)

cc: @zbeyens

@ologbonowiwi
Copy link
Author

@zbeyens let me know if it fills the expectations or there's anything that needs to be changed 😄

@@ -366,6 +374,14 @@ export const withReact = <T extends BaseEditor>(
onContextChange()
Copy link
Contributor

@zbeyens zbeyens Oct 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to pass a parameter to this function as stated in #5521 (comment)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not getting what you mean. What should I pass as parameter here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need 2 new props to Slate: onSelectionChange and onValueChange. These callbacks should be called in onContextChange like onChange.

@zbeyens
Copy link
Contributor

zbeyens commented Oct 17, 2023

Could you add unit tests for:

  • Slate prop: onValueChange
  • Slate prop: onSelectionChange
  • editor method: editor.onValueChange
  • editor method: editor.onSelectionChange

@ologbonowiwi
Copy link
Author

Where should I add the tests?

@zbeyens
Copy link
Contributor

zbeyens commented Oct 17, 2023

In packages/slate-react/test and packages/slate/test

@@ -23,6 +23,7 @@ export const Slate = (props: {
children: React.ReactNode
onChange?: (value: Descendant[]) => void
}) => {
// do we need to add onValueChange and onSelectorChange here?
Copy link
Author

@ologbonowiwi ologbonowiwi Oct 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is where I need to add the new handlers, @zbeyens?

and execute them inside onContextChange?

or do you want me to add a new parameter on onContextChange, where we'll send the onValueChange/onSelectorChange (from this line)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever that makes it work :) Will be confirmed by unit tests.

@zbeyens
Copy link
Contributor

zbeyens commented Oct 19, 2023

Another contributor has submitted a complete and satisfactory PR #5526. Therefore, this one can be closed. Thank you for your efforts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Derive onSelectionChange and onValueChange from onChange
2 participants